Skip to content

Fix memory leak in daemon_unpackapplyfilter() error paths#1640

Open
nidu-ninja wants to merge 1 commit intothe-tcpdump-group:masterfrom
nidu-ninja:rpcapd-error-path-leak
Open

Fix memory leak in daemon_unpackapplyfilter() error paths#1640
nidu-ninja wants to merge 1 commit intothe-tcpdump-group:masterfrom
nidu-ninja:rpcapd-error-path-leak

Conversation

@nidu-ninja
Copy link
Copy Markdown

Fix a memory leak in daemon_unpackapplyfilter() where dynamically
allocated BPF instruction memory was not freed on early-return
error paths.

This change introduces a structured cleanup block to ensure
allocated memory is released when instruction reception,
validation, or filter application fails.

@infrastation
Copy link
Copy Markdown
Member

Please put the commit message into the commit rather than request description and keep the existing indentation style.

@nidu-ninja nidu-ninja force-pushed the rpcapd-error-path-leak branch from 7b563c7 to b76ffe6 Compare March 6, 2026 21:06
@infrastation
Copy link
Copy Markdown
Member

For reference, this problem has been known as Coverity CID 1641537.

Comment thread rpcapd/daemon.c
static int
daemon_unpackapplyfilter(PCAP_SOCKET sockctrl, SSL *ctrl_ssl, struct session *session, uint32_t *plenp, char *errmsgbuf)
{
int status;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @infrastation said, please don't change the indentation style - it makes it difficult to see what the real changes are.

@fxlb
Copy link
Copy Markdown
Member

fxlb commented Mar 27, 2026

Avoid merge, use rebase on top of master.

@nidu-ninja nidu-ninja force-pushed the rpcapd-error-path-leak branch from 7dc5fea to d78dff8 Compare March 27, 2026 17:55
Comment thread CHANGES Outdated
(EOA) as of December 31, 2017
Make pcap_compile() error messages more uniform and consistent.
Deprecate pcap_compile_nopcap().
Deprecate bpf_filter().
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a change for 1.10.7, not 1.11.0.

Comment thread CHANGES Outdated
Fix DECnet packet filtering on big-endian hosts.
Fix various failures to reject invalid DECnet primitives.
Require "vpi" and "vci" values to be within valid ranges.
Initialize the scratch memory store to 0.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change that you should be making to the CHANGES file.

Comment thread CHANGES
In "gateway" negate the host(s), but not the protocol.
Reject "gateway" within MPLS, VXLAN or Geneve.
In "net <n> mask <m>" catch ENOMEM for the "m" too.
Match both byte orders for the AF_ value when filtering DLT_NULL
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be removed.

Comment thread CHANGES Outdated
Discuss Linux BPF extensions in the man pages.
Note endianness in pcap_compile(3PCAP) and pcap_lookupnet(3PCAP).
man: Document devices, interfaces and "any" better.
Remove list of OSes that support "ipv6-icmp"; all the ones we
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another 1.10.7 change that should not be added here.

Comment thread CHANGES
Remove list of OSes that support "ipv6-icmp"; all the ones we
support appear to do so.
Add a README.qnx.md file.
Improve dcumentation of TLS supprt in rpcapd man page.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove existing CHANGES entries.

Comment thread CHANGES
QNX:
Disable zero-copy BPF to work around portability issues.
Use "unix.h" instead of the missing <sysexits.h>.
RDMA:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove existing CHANGES entries and don't remove the 1.10.7 set of changes.

@guyharris
Copy link
Copy Markdown
Member

It appears that you don't have the current version of the CHANGES file as the baseline for your change. Please fix that.

@fxlb
Copy link
Copy Markdown
Member

fxlb commented Mar 27, 2026

Whitespaces change problem. Please fix.

@nidu-ninja nidu-ninja force-pushed the rpcapd-error-path-leak branch 2 times, most recently from fdbdd17 to 6480125 Compare March 30, 2026 16:17
@infrastation
Copy link
Copy Markdown
Member

After the clean-ups the code makes much more sense, thank you. Please wrap the commit message to 72 characters, also there is no need to state the obvious ("Keep existing daemon.c indentation style and make only the relevant CHANGES entry for this fix").

@guyharris
Copy link
Copy Markdown
Member

Fix a memory leak in daemon_unpackapplyfilter() where dynamically allocated BPF instruction memory was not freed on early-return error paths.

It wasn't freed on any return paths, ao it would leak memory even on success (when pcap_setfilter() returns, either the filter has been copied into kernel space or has been copied to a newly-allocated user-mode structure, so, if the tilter code has been allocated, it can and must be freed). Your change fixes that as well.

So it should say

Fix a memory leak in daemon_unpackapplyfilter() where dynamically allocated BPF instruction memory was not freed before returning.

@guyharris
Copy link
Copy Markdown
Member

And the second paragraph just gives the details of how the leak was fixed; it can either be removed or changed to be

This change introduces a structured cleanup block to ensure allocated memory is released.

Fix a memory leak in daemon_unpackapplyfilter() where dynamically
allocated BPF instruction memory was not freed before returning.
Reported as Coverity CID 1641537.
@nidu-ninja nidu-ninja force-pushed the rpcapd-error-path-leak branch from 6480125 to 8ef2fa3 Compare March 30, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants